-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misc Python update / signal / child-workflow features #361
Conversation
Added all my non-typescript outstanding changes to this PR, so it now contains
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor things, gonna have to wait until update releases of course (hopefully soon)
CHILD_WORKFLOW_INPUT = "child-input" | ||
CHILD_WORKFLOW_INITIAL_STATE = "1" | ||
SIGNAL_ARG = "2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have to change, but for me extracting constants out in these tiny tests doesn't have much value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree I had too many constants. I think I still like one or two of them where they are repeated several times and critical to test logic.
@workflow.defn | ||
class ChildWorkflow: | ||
def __init__(self) -> None: | ||
self._state = CHILD_WORKFLOW_INITIAL_STATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._state = CHILD_WORKFLOW_INITIAL_STATE | |
self._state: Optional[str] = None |
Instead of constants, but this is pedantic, don't have to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, agree that's a cleaner form for the initial state. Taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to apply to other features in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (but has to wait on Python release to merge of course)
Thanks for reviewing. I've converted this to draft as a marker that it can't be merged yet. |
5087cbb
to
918ab3d
Compare
a5998b4
to
3ab582e
Compare
No description provided.